Repository navigation
fix(mcp): support MCP 1 and 2 tool exposure - #4567
Conversation
|
/ok to test 2b6ff6d |
|
/review |
svcnvidia-nemo-ci
left a comment
There was a problem hiding this comment.
SHIP
LGTM — no reliability concerns. Reviewed all four changed files and traced the affected registration, session-token enforcement, dispatch, serialization, dependency, and lifespan paths. No actionable defects found.
Static review only; tests were not executed in this environment.
Note: formal approval is not available for this request, so this review is posted as a comment only.
Review generated by codex
ananthsub
left a comment
There was a problem hiding this comment.
thanks for the contribution!
|
Thanks fort the review @ananthsub ! Could you authorise the CI again? Thanks! |
Signed-off-by: thibautbar <t.barroyer@icloud.com>
Signed-off-by: thibautbar <t.barroyer@icloud.com>
Signed-off-by: thibautbar <t.barroyer@icloud.com>
0bbc65f to
954a6f5
Compare
|
/ok to test 954a6f5 |
|
/ok to test 3010e4e |
|
🔄 Merge queue validation started! You can track the progress here: https://github.com/NVIDIA-NeMo/Gym/actions/runs/40527848937 |
What does this PR do?
Support MCP Python SDK 1.x and 2.x in Gym’s automatic tool exposure, and widen the dependency constraint from mcp>=1.28.1,<2 to mcp>=1.28.1,<3.
This removes an MCP dependency conflict when installing Gym alongside recent vLLM releases: vLLM 0.30.0 and 0.31.0 require mcp>=2.0.0,<3.0.0, which cannot resolve with Gym’s previous <2 constraint.
This matters for NeMo RL integrations that install Gym and vLLM together. NeMo RL currently pins vLLM 0.29.0 and includes Gym as an editable dependency. This change removes the MCP constraint blocking an upgrade to newer vLLM releases; it does not upgrade NeMo RL’s vLLM pin or establish compatibility for the entire training stack.
Validation
Validated on
dev-thibaut-2with Python 3.13.14:python -m pytest tests/unit_tests/test_mcp_auto_exposure.py responses_api_agents/osworld_agent/tests/test_dependency_policy.py -q: 68 passed with MCP 1.29.0, and 68 passed with MCP 2.3.0.gym env test +entrypoint=resources_servers/ns_tools +should_validate_data=true: 20 passed; data validation passed.gym env test +entrypoint=resources_servers/newton_bench +should_validate_data=true: 61 passed; data validation passed.gym env test +entrypoint=responses_api_agents/hermes_agent +should_validate_data=true: 305 passed, including the previously failing MCP connection and seeded-session tool call.gym env test +entrypoint=responses_api_agents/osworld_agent +should_validate_data=true: 384 passed.pre-commit run --all-files: passed; private guardwords catalog unavailable, so that scan was skipped.git diff --check: passed.Full local CI-equivalent run
Executed the repository CI scripts on
dev-thibaut-2, using pinned uv 0.11.29 and Python 3.13.14, starting fromd9b901d0and applying the GDPVal test fix for its rerun:scripts/ci/server_tests.sh <shard> 8for all eight shards: all 234 modules executed, with data validation. Initial JUnit reports contain 14,734 tests, 51 skipped, 8 failures and 9 errors across four modules. All four module suites passed on rerun, as detailed below.scripts/ci/core_unit_tests.sh: 8,883 passed, 28 skipped and 8 failures on the initial run. Seven failures were process-supervisor timing cases; the complete unchanged file passed on rerun (51 passed). The base checkout also exhibited timing failures in this file (2 failed, 49 passed). The remaining failure wasRuntimeError: Set changed size during iterationinside tqdm while running a rollout-reverification test; that exact test passed on isolated rerun.--cov-appendand the repository coverage threshold: 926 passed, 6 skipped. Combined coverage: 95.09%, satisfying the 95% gate.pre-commit run --all-filesandgit diff --check: passed after the final change; the private guardwords scan was unavailable.Follow-ups from the full run:
inputSchema. It now checks the serialized wire alias shared by MCP 1 and 2, preserving its comparison against the benchmark's actual tool definitions. Full GDPVal suite: 851 passed, 1 skipped with MCP 2.3.0;tests/test_tools.py: 10 passed with MCP 1.30.0.libXrender.so.1. After installing the host library: 11 passed.This was not a clean first-pass run: all observed failures have passing reruns, and the timing/tqdm failures remain worth monitoring in GitHub CI. No tests were disabled or assertions weakened.
During validation, the PR received merge
26ad1d6c, containing only the upstream module-logger change (#4748). The GDPVal fix was reapplied on top. On this final tree, global-config/MCP tests passed (286), registry/discovery tests passed (67), andpre-commit run --all-filespassed. The full suite was not repeated after that logging-only merge.Rollouts
The Hermes integration test exercised real HTTP/MCP transport and the pinned Hermes client against a deterministic local model stub. A real-model smoke rollout was not run: this checkout has no configured model endpoint or credentials. Full repository CI remains to be rerun on the updated head.
Compatibility and benchmark impact
MCP 1 remains supported, and MCP 2 becomes an allowed dependency for Gym. The pinned NeMo-Skills and Hermes clients still require MCP 1, so their isolated environments explicitly constrain
mcp<2. The Hermes sandbox installer applies the same constraint and checks that its required Streamable HTTP client API imports before reusing a runtime.OSWorld now permits
cryptography, matching Gym's resolver policy and MCP 2's import requirements. NewtonBench explicitly declares its directpython-dotenvdependency instead of relying on optional transitive dependencies. These fixes address the server-suite failures observed in run 38708851719. No task, verifier, or reward definition changes are intended.No separate issue is needed: this is a focused dependency-compatibility fix and follow-up to the PR's CI failures.
Checklist
pre-commit run --all-files) (so CI lint/format checks pass).git commit -s) (so the DCO check passes).